Skip to content

Run all miniphase transforms at group end #3350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Oct 26, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 19, 2017

This avoids having to change the phase of the current context on each node visit. The performance tests indicate that this gives a performance improvement of 6-7% on dotty itself.

@odersky
Copy link
Contributor Author

odersky commented Oct 20, 2017

Won't this prevent this group from passing Ycheck ?

Seems not. Ycheck still passes the group where restoreScopes used to be run.

@odersky odersky force-pushed the change-transform-phases-3 branch from 81b61de to 5afc086 Compare October 20, 2017 08:18
@DarkDimius
Copy link
Contributor

The last phase block is not ychecked. This will mean that flatten and RestoreScopes are no longer checked. FTR ycheck used to find many bugs in those phases.

@odersky
Copy link
Contributor Author

odersky commented Oct 20, 2017

If we suspect Flatten is wrong, we could always split it into its own group and Ycheck after it. But it won't run by default, needs manual intervention.

@odersky
Copy link
Contributor Author

odersky commented Oct 20, 2017

test performance please

@dottybot
Copy link
Member

performance test scheduled: 2 job(s) in queue, 1 running.

@smarter
Copy link
Member

smarter commented Oct 20, 2017

The last phase block is not ychecked.

@DarkDimius I think it is, LabelDefs is in the last group: https://github.com/lampepfl/dotty/blob/7c8f55741f7f85c26116357705b7db3e71739dfd/compiler/test/dotty/tools/vulpix/TestConfiguration.scala#L45

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3350/ to see the changes.

Benchmarks is based on merging with master (6d7b9c0)

@odersky odersky changed the title [WIP] Try to run most phases at group end Run all miniphase transforms at group end Oct 21, 2017
Only exception is RefChecks which produces errors when run
at end of group. Probably because it clashes with ExtensionMethods.
Only exception: elimByName, which produces errors if run at group end.
LambdaLift cannot run at same phase as Flatten, so it's better to move
Flatten one phase later.

Some check files that dependent on the order in which symbols were accessed
needed to be updated.
The other phase, LinkScala2Impls cannot yet be run at group end - it produces
errors if one tries.
Instead of restoreScopes, use elimStaticThis, which is the new
end of the group.
If all phases run at group end, any phase that adds or removes method parameters
makes all phases run before it in the same group have the wrong number of parameters
when type assigning an Apply or TypeApply. Previously this was allowed only if
relaxedTyping was true. Now it is allowed everywhere. We should still be able to
catch any errors in the transformer logic using Ycheck.

With this change, LinkScala2Impls can now be run at group end.
Move to end of group. Needed a fix in ExtensionMethods to work
correctly. Before the fix, extension methods got wrong flags.
We now have all miniphases running at group end. We can remove the infrastructure
to enable other behavior. If some parts of some transformations need to run at a different
phase than group end, we can always to `ctx.atPhase { ... }`.
It's no longer needed, since it's the default now.
Got deleted by accident before.
@odersky odersky force-pushed the change-transform-phases-3 branch from bcfafe1 to 3a0184c Compare October 21, 2017 07:26
TreeTransform used cpyBetweenPhases, but TreeTransformer did not.
This does not seem sensical. Most likely this was overlooked. TreeTransform/TreeTransformer
is too similar, one gets confused easily what is what.
@odersky odersky mentioned this pull request Oct 22, 2017
@odersky odersky requested review from DarkDimius and smarter October 23, 2017 10:04
@smarter
Copy link
Member

smarter commented Oct 23, 2017

Would be interesting to get some statistics on how much this reduces the number of Context objects we create.

@smarter
Copy link
Member

smarter commented Oct 23, 2017

test performance please

@dottybot
Copy link
Member

performance test scheduled: 2 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3350/ to see the changes.

Benchmarks is based on merging with master (e1365d9)


/** Renames lifted classes to local numbering scheme */
class RenameLifted extends MiniPhaseTransform with SymTransformer { thisTransformer =>

override def phaseName = "renameLifted"

override def runsAfterGroupsOf: Set[Class[_ <: Phases.Phase]] = Set(classOf[RestoreScopes])
// Not clear why this should run after restoreScopes
// override def runsAfterGroupsOf: Set[Class[_ <: Phases.Phase]] = Set(classOf[RestoreScopes])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolasstucki Can you comment on that ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally it was on Flatten because this transformation acts on the flattened names. But @DarkDimius noted that the flattening transformation finishes in RestoreScopes.

@@ -471,6 +464,15 @@ object TreeTransforms {

def miniPhases: Array[MiniPhase]

private var myGroupEndId: PhaseId = NoPhaseId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be private[this] :)

@@ -404,7 +406,7 @@ trait TypeAssigner {
}
else {
val argTypes = preCheckKinds(args, pt.paramInfos).tpes
if (sameLength(argTypes, paramNames) || ctx.phase.prev.relaxedTyping) pt.instantiate(argTypes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it'd be nice to keep the relaxedTyping check when we can (so, at least in the Typer).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll roll that into the next PR (#3366).

@odersky odersky merged commit 1ee4d1e into scala:master Oct 26, 2017
@allanrenucci allanrenucci deleted the change-transform-phases-3 branch December 14, 2017 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants